Skip to content

Conversation

martakam
Copy link
Member

@martakam martakam commented Sep 16, 2026

This PR adds filtering for books based on multiple attributes.

Filter books based on:

  • Author name
  • Subject
  • Place (location)
  • Person
  • Language
  • Publisher

To be merged after #27.

@martakam martakam linked an issue Sep 16, 2026 that may be closed by this pull request
@martakam martakam self-assigned this Sep 16, 2026
@martakam martakam added the enhancement New feature or request label Sep 16, 2026
@martakam martakam marked this pull request as ready for review September 17, 2026 16:10
@martakam martakam requested a review from a team as a code owner September 17, 2026 16:10
@martakam
Copy link
Member Author

This PR adds a lot of functionality, so it should be tested thoroughly.
Also, please comment whether the question mark icon used besides the "Language" filter is understandable enough in order to explain what that filter really does, or if we should do something that is a little easier to see.

@hestro hestro self-requested a review September 17, 2026 21:10
Copy link
Member

@hestro hestro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a thorough review, but I noticed a few things while taking a quick look:

When adding a few filters to a search, then closing the filter tab and reopening it, the filters are no longer shown in the fields - but they are still active, and there is no way to remove them from the search without refreshing the page.

Additionally, according to the project description, filter selections should be remembered even if the page is refreshed.

@hestro
Copy link
Member

hestro commented Sep 17, 2026

Also, please comment whether the question mark icon used besides the "Language" filter is understandable enough in order to explain what that filter really does, or if we should do something that is a little easier to see.

I think the question mark works, but it took me a few tries before I realised I had to hover and wait. I tried clicking it first, and nothing happened. So it's understandable once you get it, but not immediately obvious.

@johahor
Copy link
Member

johahor commented Sep 18, 2026

Also, please comment whether the question mark icon used besides the "Language" filter is understandable enough in order to explain what that filter really does, or if we should do something that is a little easier to see.

I think the question mark works, but it took me a few tries before I realised I had to hover and wait. I tried clicking it first, and nothing happened. So it's understandable once you get it, but not immediately obvious.

Agree. Placement is fine, but just make it appear straight away when hovering and it's fine.

Nevermind, apparently this is a limitation on the browser itself. However, a visible line of helper text under "Language" would be simpler and work everywhere.

@johahor
Copy link
Member

johahor commented Sep 18, 2026

Since this PR adds a lot of functionality and covers different requirements, i figured i'd let Claude have a crack at reviewing (also it says in project 1 to use AI for quality control so this fits nicely). Here is the response:

Commit 767901c (use sessionStorage to save filters) is a step in the right direction, but it only solves half of the problem.

What it does now
FilterBooksSection writes the filters to sessionStorage on every change and reads them back when it mounts. That is why the fields show the previous values again after closing and reopening the filter panel, and after a reload.

What is still missing
The active search does not read from sessionStorage. BookGrid owns the filters in its search state, and that state always starts as { query: "", filters: {} }. So whenever BookGrid mounts (reload, or opening a book and going back), the results are unfiltered even though the fields show a filter. I reproduced it with place: New York: after a reload the field says "New York", sessionStorage has it, but the grid shows the default fantasy list (1 of 9936).

How to fix it, and how it relates to #32
The root cause is that there are two owners of the filter state: FilterBooksSection (sessionStorage + local state) and BookGrid (the state that is actually used for the search). #32 ran into the same class of problem with the search text: it was lost when opening a book, because BookGrid unmounts. The fix there was to move searchQuery and the useBooks call up to App, which is always mounted, and pass them down as props.

The filters should follow the same pattern. After #32 the state owner is App, so the fix is:

// App (after #32 is merged)
const [filters, setFilters] = useState<BookFilters>(() =>
  JSON.parse(sessionStorage.getItem("filters") ?? "{}"),
);

const handleFilterChange = (newFilters: BookFilters) => {
  sessionStorage.setItem("filters", JSON.stringify(newFilters));
  setFilters(newFilters);
};

const booksQuery = useBooks({ query, page, filters });

and filters + onFilterChange are passed down through BookGrid and SearchBar to FilterBooksSection, which then becomes a controlled component with no state or storage of its own, the same way SearchBar already receives query.

That fixes closing/reopening the panel, reload, and go-back in one change, and removes the mismatch between what the fields show and what is applied.

Merge order
Both PRs change the state in BookGrid.tsx, so whichever merges second gets a conflict there. I suggest merging #32 first, since it is smaller and already moves the state to where the filter state also needs to live. This PR then merges main and moves the filter state into App as above. If this PR goes first, the same change has to be done in BookGrid now and moved again in #32.

@hestro hestro self-requested a review September 18, 2026 17:55
hestro
hestro previously approved these changes Sep 18, 2026
Copy link
Member

@hestro hestro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested it and everything looks like it's working well! I like the addition of the badge showing the number of active filters!

I have also included a few minor stylistic tweaks (e.g. the search bar and filter button have a 2px height difference, which is a bit annoying).

Co-authored-by: Henrik Strøm-Andersen <hestro@stud.ntnu.no>
@martakam
Copy link
Member Author

Adjusted the height now. Just used 48px instead of 44px so the number is divisible by 16. Just a stylistic choice, nothing more than that

@martakam martakam closed this Sep 18, 2026
@martakam martakam reopened this Sep 18, 2026
@martakam martakam requested a review from hestro September 18, 2026 18:48
Copy link
Member

@hestro hestro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

@martakam martakam merged commit 3b39d29 into main Sep 18, 2026
Sign in to join this conversation on GitHub.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: filtering book search results
3 participants